Remove an Rc optimization that's no longer necessary

This optimization ended up not being correct with the recent switch to being
more recursive, and after some profiling it looks like this optimization for
memory usage isn't even needed any more. This commit removes the `Rc` sharing,
fixing #1841 in the process.

Closes #1841

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index c7c245b2c48228b78c32bab605c3528038b4aed8..0fe74b8d8f9e08913d7272fa90ba44fc278b471d 100644 (file)
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -92,7 +92,6 @@
 //! dependency graph (one of the largest known ones), so hopefully it'll work
 //! for a bit longer as well!
 
-use std::cell::RefCell;
 use std::collections::HashSet;
 use std::collections::hash_map::HashMap;
 use std::fmt;
@@ -232,7 +231,7 @@ impl fmt::Debug for Resolve {
 struct Context {
     activations: HashMap<(String, SourceId), Vec<Rc<Summary>>>,
     resolve: Resolve,
-    visited: Rc<RefCell<HashSet<PackageId>>>,
+    visited: HashSet<PackageId>,
 }
 
 /// Builds the list of all packages required to build the first argument.
@@ -244,7 +243,7 @@ pub fn resolve(summary: &Summary, method: Method,
     let cx = Box::new(Context {
         resolve: Resolve::new(summary.package_id().clone()),
         activations: HashMap::new(),
-        visited: Rc::new(RefCell::new(HashSet::new())),
+        visited: HashSet::new(),
     });
     let _p = profile::start(format!("resolving: {}", summary.package_id()));
     match try!(activate(cx, registry, &summary, method, &mut |cx, _| Ok(Ok(cx)))) {
@@ -271,14 +270,14 @@ fn activate(mut cx: Box<Context>,
     // Dependency graphs are required to be a DAG, so we keep a set of
     // packages we're visiting and bail if we hit a dupe.
     let id = parent.package_id();
-    if !cx.visited.borrow_mut().insert(id.clone()) {
+    if !cx.visited.insert(id.clone()) {
         return Err(human(format!("cyclic package dependency: package `{}` \
                                   depends on itself", id)))
     }
 
     // If we're already activated, then that was easy!
     if cx.flag_activated(parent, &method) {
-        cx.visited.borrow_mut().remove(id);
+        cx.visited.remove(id);
         return finished(cx, registry)
     }
     debug!("activating {}", parent.package_id());
@@ -292,8 +291,8 @@ fn activate(mut cx: Box<Context>,
     };
 
     activate_deps(cx, registry, parent, platform, deps.iter(), 0,
-                  &mut |cx, registry| {
-        cx.visited.borrow_mut().remove(parent.package_id());
+                  &mut |mut cx, registry| {
+        cx.visited.remove(id);
         finished(cx, registry)
     })
 }
@@ -373,7 +372,7 @@ fn activate_deps<'a>(cx: Box<Context>,
         // If we hit an intransitive dependency then clear out the visitation
         // list as we can't induce a cycle through transitive dependencies.
         if !dep.is_transitive() {
-            my_cx.visited.borrow_mut().clear();
+            my_cx.visited.clear();
         }
         let my_cx = try!(activate(my_cx, registry, candidate, method,
                                   &mut |cx, registry| {

diff --git a/tests/test_cargo_registry.rs b/tests/test_cargo_registry.rs
index 4dc750a99810dac8db2f9eac3e53e3a6973f9271..9f9426c7aa2927e39deab553ba44c8dbd0ef7771 100644 (file)
--- a/tests/test_cargo_registry.rs
+++ b/tests/test_cargo_registry.rs
@@ -771,3 +771,37 @@ test!(update_transitive_dependency {
 {compiling} foo v0.5.0 ([..])
 ", downloading = DOWNLOADING, compiling = COMPILING)));
 });
+
+test!(update_backtracking_ok {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.5.0"
+            authors = []
+
+            [dependencies]
+            webdriver = "0.1"
+        "#)
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    r::mock_pkg("webdriver", "0.1.0", &[("hyper", "0.6", "normal")]);
+    r::mock_pkg("hyper", "0.6.5", &[("openssl", "0.1", "normal"),
+                                    ("cookie", "0.1", "normal")]);
+    r::mock_pkg("cookie", "0.1.0", &[("openssl", "0.1", "normal")]);
+    r::mock_pkg("openssl", "0.1.0", &[]);
+
+    assert_that(p.cargo("generate-lockfile"),
+                execs().with_status(0));
+
+    r::mock_pkg("openssl", "0.1.1", &[]);
+    r::mock_pkg("hyper", "0.6.6", &[("openssl", "0.1.1", "normal"),
+                                    ("cookie", "0.1.0", "normal")]);
+
+    assert_that(p.cargo("update").arg("-p").arg("hyper"),
+                execs().with_status(0)
+                       .with_stdout(&format!("\
+{updating} registry `[..]`
+", updating = UPDATING)));
+});